Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: ADR - Move Rosetta to standalone repository #16394

Closed
wants to merge 2 commits into from

Conversation

bizk
Copy link
Contributor

@bizk bizk commented May 31, 2023

Description

Closes: #16276

Epic and Discussion

The goal is to move Rosetta tool under cosmos-sdk/tools/ folder to a separated repo, in order to have a better scope, a cleaner way to maintain the code and manage all Rosetta efforts.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • [*] included the correct type prefix in the PR title
  • [*] added ! to the type prefix if API or client breaking change
  • [*] targeted the correct branch (see PR Targeting)
  • [/] provided a link to the relevant issue or specification
  • [*] followed the guidelines for building modules
  • [/] included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • [*] included comments for documenting Go code
  • [*] updated the relevant documentation or specification
  • [*] reviewed "Files changed" and left comments if necessary
  • [*] confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@bizk
Copy link
Contributor Author

bizk commented May 31, 2023

FYI: @julienrbrt feel free to share your thoughts on the adr

@bizk bizk marked this pull request as ready for review May 31, 2023 20:21
@bizk bizk requested a review from a team as a code owner May 31, 2023 20:21
@github-prbot github-prbot requested review from a team, kocubinski and JeancarloBarrios and removed request for a team May 31, 2023 20:21
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What will be the repo?
  2. Does this mean as well removing the possibility to include rosetta directly in the binary?
  3. Same comment as here: refactor: ADR - Move Rosetta to standalone repository #16392 (comment), have you investigated how it will play with the vanity url? The current tags should stay available and will be hosted here, and the new tags will be pointing to the new repo.

@tac0turtle
Copy link
Member

Want to echo what Julien said. It seems there is a change in design that is not being articulated here.

Can we get an understanding of what you have in mind for rosetta? Is it standalone or does it continue to be imported into chains. There is mention of plug-in based system, what are the trade offs to doing reflection based. Currently Rosetta is being imported into the sdk so it's not entirely standalone as it's imported into the sdk and apps. We separated it into a separate go.mod for maintenance reasons but didn't put it in a separate repo so it's kept up to date with the latest sdk changes.

@bizk
Copy link
Contributor Author

bizk commented Jun 1, 2023

Hi @julienrbrt ! Thx for the observations!

  1. we would need to create a repo inside cosmos, like "rosetta" or something similar
    2 .By binary I asume you mean from simapp? I see that every chain is adding rosetta into thier nodes binary by using the exported function rosettaCmd.RosettaCommand(interfaceRegistry, appCodec) so that wouldn't change except for the import path directory.
  2. I'm not sure I'm following the relationship between the plugins and the vanity url, so if you could elaborate more I could provide a better response

@julienrbrt
Copy link
Member

julienrbrt commented Jun 1, 2023

Hi @julienrbrt ! Thx for the observations!

  1. we would need to create a repo inside cosmos, like "rosetta" or something similar
    2 .By binary I asume you mean from simapp? I see that every chain is adding rosetta into thier nodes binary by using the exported function rosettaCmd.RosettaCommand(interfaceRegistry, appCodec) so that wouldn't change except for the import path directory.
  2. I'm not sure I'm following the relationship between the plugins and the vanity url, so if you could elaborate more I could provide a better response
  1. 👍🏾
  2. Right, this makes sense, but as rosetta will support only released versions, this means we will need to remove it from simapp here, as it will be incompatible with our development version. Are you going to do some early integration as well?
  3. I had no questions about the plugins, and indeed this is unrelated to the vanity url. I am simply curious is you are going to break the go mod or keep it the same and update figure out vanity url config.

@@ -0,0 +1,67 @@
# ADR 606: Rosetta standalone
Copy link
Contributor

@alexanderbez alexanderbez Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ADR 606: Rosetta standalone
# ADR 066: Rosetta Standalone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks mate!

@bizk
Copy link
Contributor Author

bizk commented Jun 1, 2023

Hi @tac0turtle we appreciate you jumping into the discussion!

It seems there is a change in design that is not being articulated here.

What do you think is unclear or not being correctly articulated? glad to expand over your concerns

Is it standalone or does it continue to be imported into chains.

Rosetta will always run as a service independently of the app/node, It can be imported into chains binaries with the rosettaCmd.RosettaCommand(interfaceRegistry, appCodec) o separately, the only difference would eradicate on how you start that server. We believe that running Rosetta standalone removes the need to have the chain node binary and follows the objective of decoupling things.

what are the trade offs to doing reflection based.

Well we made an ADR for plugins as julien mentioned. Using reflection is desirable but due to chains running on older versions of cosmos. we believe it will take a lot of time until chains perform the upgrade, implement the optional service for reflection and adopt rosetta. Not even cosmos-hub chain node is running neither on 0.46 nor on 0.47. We prioritized usability and adoption, since the reflection solution is already implemented on hudl tool and the proposed plugin solution is straight forward / simple, we believe implementing reflection shouldn't be a big issue

Currently Rosetta is being imported into the sdk so it's not entirely standalone as it's imported into the sdk and apps.

Right, Rosetta is imported and loaded through the function mentioned before and nothing more (kudos tho whoever came with such decoupled approach), same as for other cosmos chains, so switching repos shouldn't represent a risk of breaking stuff.

The main reason (as I understand it) for rosetta being inside a node app is so it could have access to the app interface registries and codecs, but we solved that issue with the plugins things, so we don't really see a reason why the app should have rosetta inside if it can perfectly run well outside of it.

@bizk
Copy link
Contributor Author

bizk commented Jun 1, 2023

@julienrbrt
2. Right, I honestly don't see the point of having rosetta inside a binary (except for the registered inferfaces we talked about a few weeks ago, thing that we intend to solve with the plugins update) at the end of the day, puting it inside the binary is just adding an extra command to trigger it without really impacting on its functionability or configurations.
3. I don't really have a strong opinion here to be honest. I'm inclined to configuring the vanity since I feel thats the cleanest approach, we will follow whatever is the desired practice for these cases.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

I am still not sure however this is a good argument, as they all the changes that are made in roestta on main is for fitting the SDK APIs. And the working / released versions of rosetta are based on a release branch.

Such changes can cause mismatches
in dependencies, or unexpected behaviour that needs to be addressed down the line.

@tac0turtle
Copy link
Member

tac0turtle commented Jun 3, 2023

Well we made an ADR for plugins as julien mentioned. Using reflection is desirable but due to chains running on older versions of cosmos. we believe it will take a lot of time until chains perform the upgrade, implement the optional service for reflection and adopt rosetta. Not even cosmos-hub chain node is running neither on 0.46 nor on 0.47. We prioritized usability and adoption, since the reflection solution is already implemented on hudl tool and the proposed plugin solution is straight forward / simple, we believe implementing reflection shouldn't be a big issue

this doesnt really make sense to me, why does a chain need to be on 0.46 or 0.47 to use reflection? Grpc Reflection is native to grpc and is live on all chains already. Im not the biggest fan of the plugin design as it doesnt scale.

Which plugins will be supported in the repo?

We can move it to a new repo but rosetta is a low priority for us in terms of other things that need to be done. Also with moving it to a new repo i guess you are agreeing to keep it up to date at all times when releases come out right?

@bizk
Copy link
Contributor Author

bizk commented Jun 7, 2023

@julienrbrt

Rosetta connects to a cosmos node, even when the node "depends" on rosetta because it is part of the CLI, Rosetta actually consumes the node, not the other way around, we have seen that in some cases the Rosetta sub repo suffers from breaking changes on non released things due to refactors or mass implementation, we would like to avoid that

@tac0turtle

this doesnt really make sense to me, why does a chain need to be on 0.46 or 0.47 to use reflection? Grpc Reflection is native to grpc and is live on all chains already. Im not the biggest fan of the plugin design as it doesnt scale.

Well, you are right about reflection and scaling. what I meant is that hubl does the job of getting the interfaces and their respective implementation through reflection but it is not compatible with old versioning, I didn't want to repeat the efforts, but now I'm exploring on reflection to achieve it.

Which plugins will be supported in the repo?

We are working on onbiarding Cosmos-hub, Osmosis and Evmos to test different ecosystems, after that we would provide guidelines for interested zones on how to make their own plugins.

We can move it to a new repo but rosetta is a low priority for us in terms of other things that need to be done. Also with moving it to a new repo i guess you are agreeing to keep it up to date at all times when releases come out right?

The main objective behind the new repo is to keep it well maintained.

@tac0turtle
Copy link
Member

Well, you are right about reflection and scaling. what I meant is that hubl does the job of getting the interfaces and their respective implementation through reflection but it is not compatible with old versioning, I didn't want to repeat the efforts, but now I'm exploring on reflection to achieve it.

maybe for all future versions we do reflection, So anything that uses .47+ gets reflection everything before doesnt get anything. We shouldn't spend much time fixing the current state if the next state will be much better.

We are working on onbiarding Cosmos-hub, Osmosis and Evmos to test different ecosystems,

these wont be merged into the repo right? Ideally zondax time is not spent on adding plugins and other things that are needed for users. Rosetta isnt used by many.

@tac0turtle
Copy link
Member

if the plan is to build rosetta as a stand alone thing that apps dont import then this is good, but its still unclear to me which direction this is being taken. Id prefer this is something that apps dont need to import and can just start and it works

@tac0turtle
Copy link
Member

talked to @jleni. We can spin it out, the gotcha is we have to change the documenetation to mention rosetta as a proxy and there isnt a need to import it into the rootcmd anymore,

Ill close this ADR as its not needed, mainly needed a quick discussion

@tac0turtle tac0turtle closed this Jun 12, 2023
@bizk bizk deleted the zondax/rosetta/adr-standalone branch August 15, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Epic]: Rosetta migration to standalone repo
4 participants